Skip to content

feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown#391

Merged
gtsiolis merged 2 commits into
mainfrom
lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871
Jul 21, 2026
Merged

feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown#391
gtsiolis merged 2 commits into
mainfrom
lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871

Conversation

@gtsiolis

@gtsiolis gtsiolis commented Jul 16, 2026

Copy link
Copy Markdown
Member

Pressing Ctrl-C during a wrapped-tool run (lstk terraform apply, aws, cdk, sam, az, extensions) killed the child immediately instead of letting it shut down gracefully — so terraform never got to release its state lock. The tools run via exec.CommandContext with lstk's root context, which is cancelled on SIGINT/SIGTERM; exec.CommandContext's default Cancel then SIGKILLs the child right away.

All wrapped-tool passthroughs now run through a new proc.Run helper that:

  • disarms the SIGKILL-on-cancel (its Cancel returns os.ErrProcessDone), so the tool terminates from the signal it received and lstk waits for its own shutdown — preserving the real exit code (Ctrl-C on terraform now exits 130 after full cleanup instead of signal: killed/255);
  • forwards SIGTERM unconditionally: a terminal never generates it, so without forwarding kill <lstk-pid>, timeout 30 lstk …, or an IDE stop button would never reach the tool;
  • forwards SIGINT only when none of stdin/stdout/stderr is a terminal: an attached terminal already delivers Ctrl-C to the child via the foreground process group, and a second near-simultaneous SIGINT makes tools like terraform abort without cleaning up. Checking all three streams matters — with only stdin redirected (yes | lstk terraform apply) the process group still delivers;
  • registers its signal handler before Start, so a signal landing during startup is queued rather than lost.

(npm/launcher.js forwards unconditionally instead — safe there because its child is lstk itself, which tolerates duplicate signals; wrapped tools don't.)

Short internal captured-output execs (version checks, schema discovery, backend provisioning) are unchanged. No configurable grace period yet, per the issue scope.

Tests: internal/proc unit tests cover the disarm/exit-code semantics and the forwarding policy; test/integration/signal_forwarding_test.go covers the behavior end to end through a new signal-wait mode of the reference extension (exits 40 + number of signals received) — non-interactive SIGINT/SIGTERM forwarding, exactly-one SIGINT on Ctrl-C in a PTY, the piped-stdin double-signal regression, and SIGTERM reaching the tool from inside a terminal.

Fixes DEVX-1000

@gtsiolis gtsiolis added semver: patch docs: skip Pull request does not require documentation changes labels Jul 16, 2026
@gtsiolis
gtsiolis marked this pull request as ready for review July 16, 2026 17:50
@gtsiolis
gtsiolis requested a review from a team as a code owner July 16, 2026 17:50
@gtsiolis gtsiolis self-assigned this Jul 16, 2026
@gtsiolis
gtsiolis force-pushed the lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871 branch from c3b7b9c to 2f772f9 Compare July 20, 2026 07:09
gtsiolis and others added 2 commits July 21, 2026 07:07
Wrapped tools (aws, terraform, cdk, sam, az, extensions) were run via
exec.CommandContext with lstk's root context, which is cancelled on
SIGINT/SIGTERM. exec.CommandContext's default Cancel then SIGKILLs the
child immediately, so Ctrl-C during e.g. `lstk terraform apply` killed
terraform before it could clean up and release its state lock.

Route these execs through a new proc.Run helper that disarms the
SIGKILL-on-cancel (preserving the tool's real exit code) and forwards
SIGINT/SIGTERM to the child in the non-interactive case, letting the
tool shut down gracefully. Interactive Ctrl-C already reaches the child
via the controlling terminal's process group, so forwarding is
suppressed there to avoid a double signal. Mirrors npm/launcher.js.

Generated with [Linear](https://linear.app/localstack/issue/LAV-973/forward-sigintsigterm-to-wrapped-tools-before-terminating-them#agent-session-efa68cad)

Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
…nal stream

Two gaps in the TTY heuristic, both proven by failing integration tests
before the fix:

- Gating suppression on stdin alone re-armed forwarding for piped-stdin
  interactive runs (`yes | lstk terraform apply`): the terminal's
  foreground process group already delivers Ctrl-C, so the wrapped tool
  received a second SIGINT and aborted without cleanup - the exact
  stale-lock bug this branch fixes. SIGINT suppression now triggers when
  any of stdin/stdout/stderr is a terminal.

- SIGTERM is never terminal-generated, so suppressing it interactively
  meant `kill <lstk-pid>`, `timeout`, or an IDE stop button never
  reached the tool while lstk swallowed the signal and waited. SIGTERM
  now forwards unconditionally.

Also registers the signal handler before Start so a signal landing
during startup is queued instead of lost, adds end-to-end signal tests
via the reference extension's new signal-wait mode, skips sh-based unit
tests on Windows, and corrects the npm/launcher.js comparison (the
launcher forwards unconditionally; its child tolerates duplicates).

Co-Authored-By: Claude <noreply@anthropic.com>
@gtsiolis
gtsiolis force-pushed the lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871 branch from 2f772f9 to 71f8a3b Compare July 21, 2026 07:08

@skyrpex skyrpex left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me! Thanks for this PR... This change very important for some of the CLIs.

@gtsiolis
gtsiolis merged commit 80dad9c into main Jul 21, 2026
17 checks passed
@gtsiolis
gtsiolis deleted the lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871 branch July 21, 2026 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs: skip Pull request does not require documentation changes semver: patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants